-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
control/controlbase: make the protocol version number selectable. #4370
Conversation
tailcfg/tailcfg.go
Outdated
@@ -34,7 +34,7 @@ import ( | |||
// Previously (prior to 2022-03-06), it was known as the "MapRequest | |||
// version" or "mapVer" or "map cap" and that name and usage persists | |||
// in places. | |||
type CapabilityVersion int | |||
type CapabilityVersion uint16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will be worth the pain.
Just explode elsewhere if this ever goes over uint16 in size. (or write some test that fails if it goes over 64k)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched tailcfg type back to int. I kept uint16 in controlbase and controlhttp, you okay with that?
Added a test that will fail when tailcfg version exceeds 60k, as an early warning to revise our wire protocol. Also added a hard panic() in controlclient if we ever try to start a session with a >uint16 version. Hard panic because the test should give us very early warning that the panic is coming, so something's very wrong if that codepath trips.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise
c4916de
to
1c9ad7e
Compare
1c9ad7e
to
14ce4cc
Compare
This is so that we can plumb our client capability version through the protocol as the Noise version. The capability version increments more frequently than strictly required (the Noise version only needs to change when cryptographically-significant changes are made to the protocol, whereas the capability version also indicates changes in non-cryptographically-significant parts of the protocol), but this gives us a safe pre-auth way to determine if the client supports future protocol features, while still relying on Noise's strong assurance that the client and server have agreed on the same version. Currently, the server executes the same protocol regardless of the version number, and just presents the version to the caller so they can do capability-based things in the upper RPC protocol. In future, we may add a ratchet to disallow obsolete protocols, or vary the Noise handshake behavior based on requested version. Updates #3488 Signed-off-by: David Anderson <danderson@tailscale.com>
14ce4cc
to
e44caf9
Compare
This is so that we can plumb our client capability version through
the protocol as the Noise version. The capability version increments
more frequently than strictly required (the Noise version only needs
to change when cryptographically-significant changes are made to
the protocol, whereas the capability version also indicates changes
in non-cryptographically-significant parts of the protocol), but this
gives us a safe pre-auth way to determine if the client supports
future protocol features, while still relying on Noise's strong
assurance that the client and server have agreed on the same version.
Currently, the server executes the same protocol regardless of the
version number, and just presents the version to the caller so they
can do capability-based things in the upper RPC protocol. In future,
we may add a ratchet to disallow obsolete protocols, or vary the
Noise handshake behavior based on requested version.
Signed-off-by: David Anderson danderson@tailscale.com